fs: add support for async iterators to `fs.writeFile` by yagipy · Pull Request #38525 · nodejs/node

yagipy · 2021-05-03T23:12:06Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

lib/fs.js

Linkgoron · 2021-05-04T09:52:59Z

lib/fs.js

+            if (writeErr) {
+              if (isUserFd) {
+                callback(writeErr);
+              } else {
+                fs.close(fd, (err) => {
+                  callback(aggregateTwoErrors(err, writeErr));
+                });
+              }


I think that in general it would make more sense to continue iterating only after a write has finished.

Agreed. As it is this will have issues with backpressure, risking flooding the destination with pending write requests. Also, I'm missing the bit about exiting the for await if the fs.write() errors on any single iteration.

jasnell · 2021-05-04T21:36:10Z

lib/fs.js

+        );
+      }
+      fs.close(fd, callback);
+    })();


There's no catch handler on the promise being returned here.

jasnell · 2021-05-04T21:37:55Z

lib/fs.js

  }
-  // write(fd, buffer, offset, length, position, callback)
+  if (isCustomIterable(buffer)) {
+    (async () => {


It would be clearer and more maintainable to separate this out into a separate top-level function.

jasnell · 2021-05-04T21:42:18Z

lib/fs.js

+          }
+        );
+      }
+      fs.close(fd, callback);


The close here is problematic because writes may still be pending from the loop.

jasnell · 2021-05-04T21:43:38Z

test/parallel/test-fs-write-file.js

  process.nextTick(() => controller.abort());
 }
+
+{


It would be better to separate the tests for the new functionality into a separate isolated test file.

jasnell

It's a good idea but the implementation needs work.

Fixes: nodejs#38075

lib/fs.js

Linkgoron · 2021-05-08T14:55:58Z

lib/fs.js

+             if (writeErr) {
+               handleWriteAllErrorCallback(fd, isUserFd, writeErr, callback);
+             } else {
+               writeAll(fd, isUserFd, buffer, offset,


I think that there's a mistake here that also exists in the fs/promises version, where the code assumes that the whole chunk has been written but it might have only been partially written.

lib/fs.js

Fixes: nodejs#38075

lib/fs.js

yagipy · 2021-05-15T10:07:21Z

@Linkgoron
I am currently working on the following response.
Are the following mistakes feasible in testing?
#38525 (comment)

I added the following test based on this test, but I was not able to reproduce the mistake. 😓 (The current implementation will pass.)

{
  const filenameLargeBuffer = join(tmpdir.path, 'testLargeBuffer.txt');
  const largeBuffer = {
    expected: 'dogs running'.repeat(512 * 1024),
    *[Symbol.iterator]() {
      yield Buffer.from('dogs running'.repeat(512 * 1024), 'utf8');
    }
  };

  fs.writeFile(
    filenameLargeBuffer, largeBuffer, common.mustSucceed(() => {
      const data = fs.readFileSync(filenameLargeBuffer, 'utf-8');
      assert.strictEqual(largeBuffer.expected, data);
    })
  );
}

Linkgoron · 2021-05-16T14:58:58Z

@Linkgoron
I am currently working on the following response.
Are the following mistakes feasible in testing?
#38525 (comment)

I added the following test based on this test, but I was not able to reproduce the mistake. 😓 (The current implementation will pass.)
{
  const filenameLargeBuffer = join(tmpdir.path, 'testLargeBuffer.txt');
  const largeBuffer = {
    expected: 'dogs running'.repeat(512 * 1024),
    *[Symbol.iterator]() {
      yield Buffer.from('dogs running'.repeat(512 * 1024), 'utf8');
    }
  };

  fs.writeFile(
    filenameLargeBuffer, largeBuffer, common.mustSucceed(() => {
      const data = fs.readFileSync(filenameLargeBuffer, 'utf-8');
      assert.strictEqual(largeBuffer.expected, data);
    })
  );
}

It's quite difficult to reproduce, and I'm not sure in which cases write actually stops in the middle.

…File`

yagipy · 2021-05-22T22:19:47Z

@Linkgoron
Fixed an issue where chunks might be partially written, referring to #38615.

jasnell · 2021-05-24T14:53:35Z

@ronag @mcollina ... would definitely appreciate you taking a look at this one.

ronag

There is a lot going on here and I’m not sure I fully agree with all of it. I’m a little bit overloaded the next few days. I won’t block this but would appreciate if we could wait a few days before landing this.

mcollina

Good work! I've left a few notes on the implementation.

mcollina · 2021-05-24T15:12:52Z

lib/fs.js

+    writeAllCustomIterable(
+      fd, isUserFd, buffer, offset, length, signal, encoding, callback)
+      .catch((reason) => {
+        handleWriteAllErrorCallback(fd, isUserFd, reason, callback);


I would schedule this via process.nextTick. If this throws synchronously for whatever reason, it will lead to an unhandled rejection.

mcollina · 2021-05-24T15:15:26Z

lib/fs.js

  });
 }

+async function writeAllCustomIterable(


Do not use async with a callback. Mixing those two can only lead to bugs. I would recommend having a promisified version of fs.write() and just use async/await.

@mcollina Thanks for your suggestion 👍 . I'm curious about the advantages of having a promisified version of fs.write() and using async/await. Could you elaborate on why this approach might be better than mixing async with a callback? I'm interested in understanding the potential benefits and how it could prevent bugs.

github-actions bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels May 3, 2021

yagipy force-pushed the add-support-for-async-iterators-to-fs-writeFile branch from 675ffcd to 9a563a1 Compare May 3, 2021 23:17

yagipy commented May 4, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

yagipy marked this pull request as ready for review May 4, 2021 04:48

Ayase-252 reviewed May 4, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

yagipy force-pushed the add-support-for-async-iterators-to-fs-writeFile branch from 9a563a1 to c9bba3f Compare May 4, 2021 08:33

yagipy requested a review from Ayase-252 May 4, 2021 08:39

Linkgoron reviewed May 4, 2021

View reviewed changes

jasnell reviewed May 4, 2021

View reviewed changes

lib/fs.js Outdated

);

}

fs.close(fd, callback);

})();

Copy link

Member

jasnell May 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no catch handler on the promise being returned here.

jasnell reviewed May 4, 2021

View reviewed changes

lib/fs.js Outdated

}

);

}

fs.close(fd, callback);

Copy link

Member

jasnell May 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The close here is problematic because writes may still be pending from the loop.

jasnell reviewed May 4, 2021

View reviewed changes

jasnell requested changes May 4, 2021

View reviewed changes

yagipy force-pushed the add-support-for-async-iterators-to-fs-writeFile branch 5 times, most recently from a7cc927 to 3313df8 Compare May 8, 2021 13:51

fs: add support for async iterators to fs.writeFile

785d76d

Fixes: nodejs#38075

yagipy force-pushed the add-support-for-async-iterators-to-fs-writeFile branch from 3313df8 to 785d76d Compare May 8, 2021 13:57

Linkgoron reviewed May 8, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

Linkgoron reviewed May 8, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

Linkgoron reviewed May 8, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

Linkgoron reviewed May 8, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

yagipy force-pushed the add-support-for-async-iterators-to-fs-writeFile branch 2 times, most recently from ba733d5 to ccdb3d1 Compare May 9, 2021 02:17

fixup! fs: add support for async iterators to fs.writeFile

6f21783

Fixes: nodejs#38075

yagipy force-pushed the add-support-for-async-iterators-to-fs-writeFile branch from ccdb3d1 to 6f21783 Compare May 9, 2021 02:50

Linkgoron reviewed May 9, 2021

View reviewed changes

lib/fs.js Show resolved Hide resolved

fixup! fixup! fs: add support for async iterators to fs.writeFile

c0b87d6

yagipy force-pushed the add-support-for-async-iterators-to-fs-writeFile branch from 176573e to c0b87d6 Compare May 15, 2021 05:17

yagipy requested a review from Linkgoron May 15, 2021 08:12

fixup! fixup! fixup! fs: add support for async iterators to `fs.write…

05a5728

…File`

yagipy requested a review from jasnell May 17, 2021 20:59

ronag reviewed May 24, 2021

View reviewed changes

mcollina requested changes May 24, 2021

View reviewed changes

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Uh oh!

Conversation

yagipy commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Linkgoron May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yagipy commented May 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Linkgoron commented May 16, 2021

Uh oh!

yagipy commented May 22, 2021

Uh oh!

jasnell commented May 24, 2021

Uh oh!

ronag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yagipy commented May 3, 2021 •

edited

Loading

Linkgoron May 8, 2021 •

edited

Loading

yagipy commented May 15, 2021 •

edited

Loading

ronag left a comment •

edited

Loading